Skip to content

Conversation

@emaxx-google
Copy link
Contributor

@emaxx-google emaxx-google commented Jan 9, 2025

Don't suggest to comment-out the parameter name if the parameter has an attribute that's spelled after the parameter name.

This prevents the parameter's attributes from being wrongly applied to the parameter's type.

This fixes #122191.

@llvmbot
Copy link
Member

llvmbot commented Jan 9, 2025

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Maksim Ivanov (emaxx-google)

Changes

Don't suggest to comment-out the parameter name if the parameter has an attribute that's spelled after the parameter name.

This prevents the parameter's attributes from being wrongly applied to the parameter's type (#122191).


Full diff: https://github.com/llvm/llvm-project/pull/122286.diff

2 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp (+16)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters.cpp (+5)
diff --git a/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp b/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
index c2d9286312dc4a..e7dfcd9859f64d 100644
--- a/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -26,6 +26,17 @@ bool isOverrideMethod(const FunctionDecl *Function) {
     return MD->size_overridden_methods() > 0 || MD->hasAttr<OverrideAttr>();
   return false;
 }
+
+bool hasAttrAfterParam(const clang::SourceManager *SourceManager,
+                       const ParmVarDecl *Param) {
+  for (const auto *Attr : Param->attrs()) {
+    if (SourceManager->isBeforeInTranslationUnit(Param->getLocation(),
+                                                 Attr->getLocation())) {
+      return true;
+    }
+  }
+  return false;
+}
 } // namespace
 
 void UnusedParametersCheck::registerMatchers(MatchFinder *Finder) {
@@ -189,6 +200,11 @@ void UnusedParametersCheck::check(const MatchFinder::MatchResult &Result) {
     if (Param->isUsed() || Param->isReferenced() || !Param->getDeclName() ||
         Param->hasAttr<UnusedAttr>())
       continue;
+    if (hasAttrAfterParam(Result.SourceManager, Param)) {
+      // Due to how grammar works, attributes would be wrongly applied to the
+      // type if we remove the preceding parameter name.
+      continue;
+    }
 
     // In non-strict mode ignore function definitions with empty bodies
     // (constructor initializer counts for non-empty body).
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters.cpp
index a3fcf30f273ef7..86b00ee56c3ce0 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/unused-parameters.cpp
@@ -33,6 +33,10 @@ void f(void (*fn)()) {;}
 // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: parameter 'fn' is unused [misc-unused-parameters]
 // CHECK-FIXES: {{^}}void f(void (* /*fn*/)()) {;}{{$}}
 
+int *k([[clang::lifetimebound]] int *i) {;}
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: parameter 'i' is unused [misc-unused-parameters]
+// CHECK-FIXES: {{^}}int *k({{\[\[clang::lifetimebound\]\]}} int * /*i*/) {;}{{$}}
+
 // Unchanged cases
 // ===============
 void f(int i); // Don't remove stuff in declarations
@@ -41,6 +45,7 @@ void h(int i[]);
 void s(int i[1]);
 void u(void (*fn)());
 void w(int i) { (void)i; } // Don't remove used parameters
+int *x(int *i [[clang::lifetimebound]]) { return nullptr; } // Don't reanchor attribute to the type.
 
 bool useLambda(int (*fn)(int));
 static bool static_var = useLambda([] (int a) { return a; });

Don't suggest to comment-out the parameter name if the parameter has an
attribute that's spelled after the parameter name.

This prevents the parameter's attributes from being wrongly applied to
the parameter's type.
@emaxx-google emaxx-google force-pushed the clang-tidy-unused-param-with-attr branch from 187db18 to 3faba8c Compare January 9, 2025 15:06
@emaxx-google
Copy link
Contributor Author

@usx95

@usx95 usx95 self-requested a review January 9, 2025 15:49
@emaxx-google emaxx-google requested a review from usx95 January 9, 2025 17:44
Copy link
Contributor

@usx95 usx95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM.

@usx95 usx95 merged commit 1b897f7 into llvm:main Jan 9, 2025
6 of 7 checks passed
BaiXilin pushed a commit to BaiXilin/llvm-project that referenced this pull request Jan 12, 2025
…2286)

Don't suggest to comment-out the parameter name if the parameter has an
attribute that's spelled after the parameter name.

This prevents the parameter's attributes from being wrongly applied to
the parameter's type.

This fixes llvm#122191.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

clang-tidy misc-unused-parameters breaks attributes on parameters

3 participants